-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow mutation through a transform_iterator #2006
base: main
Are you sure you want to change the base?
Conversation
ec3c5ae
to
6b6d930
Compare
🟩 CI finished in 5h 20m: Pass: 100%/250 | Total: 5d 02h | Avg: 29m 26s | Max: 1h 02m | Hits: 54%/248341
|
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
+/- | Thrust |
CUDA Experimental | |
pycuda |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
+/- | CUB |
+/- | Thrust |
CUDA Experimental | |
+/- | pycuda |
🏃 Runner counts (total jobs: 250)
# | Runner |
---|---|
178 | linux-amd64-cpu16 |
41 | linux-amd64-gpu-v100-latest-1 |
16 | linux-arm64-cpu16 |
15 | windows-amd64-cpu16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We should figure out and document the edge cases for how this feature behaves for cross-system vectors/references.
- A new thrust example would be a nice-to-have.
- The transform_iterator should document allowed and disallowed usages of this feature.
thrust/testing/transform_iterator.cu
Outdated
void TestTransformIteratorAsDestination() | ||
{ | ||
constexpr auto n = 10; | ||
thrust::host_vector<int> src(n, 1234); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Also test with device_vector
. The CUDA-backed device_vector
references/pointers/etc are significantly more complex than the host_vector
implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that opened a can of worms
_CCCL_HOST_DEVICE typename super_t::reference dereference() const | ||
{ | ||
// TODO(bgruber): I am not sure this is the correct check here. There is also the trait | ||
// thrust::detail::is_wrapped_reference that sounds fitting. Only allowing to pass through l-value references |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, the device_vector
references are proxy references that is_wrapped_reference
is designed to detect. We may need to handle these specially.
thrust::host_vector<int> src(n, 1234); | ||
thrust::host_vector<foo> dst(n, foo{1, 2}); | ||
|
||
thrust::copy(src.begin(), src.end(), thrust::make_transform_iterator(dst.begin(), access_x{})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: What happens if this is attempted on cross-system host<->device vectors?
This feature requires some more changes to improve support for proxy references (we currently force a proxy reference to decay into its value type and pass that further). Fixing that breaks some "actors", whatever that is. I'll see when I have some time to give it another go. |
6b6d930
to
5115363
Compare
5115363
to
9785d2e
Compare
But only if the transform iterator's base iterator does not return a wrapped reference and is not a device_vector
9785d2e
to
22f3939
Compare
But only if the transform iterator's base iterator returns a true l-value reference (and not a proxy reference).